fix: improve handling for invalid responses#1283
Conversation
Noroth
requested review from
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughAdds federated-response validation during gRPC datasource loads, enforces strict batched-merge counts via a new batchStats mechanism in the resolver, and introduces a Warehouse entity/proto/mappings plus tests that exercise mismatched batch-return errors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
100-128: Classic goroutine capture issue: index and invocation must be re-bound inside the loop.As written, the closure captures the same variables, leading to racy writes to responses[...] and wrong method inputs under concurrency.
Apply this minimal fix:
- for index, invocation := range invocations { - errGrp.Go(func() error { + for index, invocation := range invocations { + index, invocation := index, invocation + errGrp.Go(func() error { a := astjson.Arena{} // Invoke the gRPC method - this will populate invocation.Output methodName := fmt.Sprintf("/%s/%s", invocation.ServiceName, invocation.MethodName) @@ - mu.Lock() - defer mu.Unlock() + mu.Lock() + defer mu.Unlock() @@ - responses[index] = response + responses[index] = response return nil }) }Optional follow-up: if jsonBuilder is thread-safe (it appears to be read-only across goroutines), consider removing the mutex or narrowing it to just the assignment to responses[index] (which is safe without a mutex since each index is unique). This would improve throughput.
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/json_builder.go (1)
119-153: Typo: entitiyCountPerType → entityCountPerType.Minor but worth fixing for readability.
- entitiyCountPerType := make(map[string]int) + entityCountPerType := make(map[string]int)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
119-125: Surface federated-validation failures as FailedPrecondition, not INTERNAL.builder.writeErrorBytes maps status errors to extensions.code. Since validateFederatedResponse is a precondition check on downstream data shape, FailedPrecondition is a better fit. If you don’t adopt status codes inside validateFederatedResponse, wrap here.
Two options (pick one):
If validateFederatedResponse already returns a status error (preferred after the proposed refactor), keep as-is.
Otherwise, wrap the error here:
- err = builder.validateFederatedResponse(response) - if err != nil { - return err - } + if err := builder.validateFederatedResponse(response); err != nil { + return status.Errorf(codes.FailedPrecondition, "invalid federated subgraph response: %v", err) + }Ensure imports:
+ "google.golang.org/grpc/codes" + "google.golang.org/grpc/status"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/json_builder.go(1 hunks)v2/pkg/engine/resolve/loader.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.gov2/pkg/engine/datasource/grpc_datasource/json_builder.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Success
🔇 Additional comments (1)
v2/pkg/engine/resolve/loader.go (1)
956-962: Good addition: explicit error text for batch-size mismatches.The new error message is clear and actionable. Once the above logic issue is addressed, this constant will serve both correctness and debuggability well.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/resolve/loader.go (1)
636-650: Guard against out-of-range accesses during merge; replace magic -1 with a sentinel constant.
batch[item]can panic ifitemis out of range. Also,-1sentinel should be a named constant to improve readability and avoid mistakes.Apply this diff:
- for i, stats := range res.batchStats { - for _, item := range stats { - if item == -1 { - continue - } - items[i], _, err = astjson.MergeValuesWithPath(items[i], batch[item], res.postProcessing.MergePath...) + for i, stats := range res.batchStats { + for _, idx := range stats { + if idx == batchIndexSkipped { + continue + } + if idx < 0 || idx >= len(batch) { + return l.renderErrorsFailedToFetch(fetchItem, res, fmt.Sprintf(invalidBatchIndexOutOfRange, idx, len(batch))) + } + items[i], _, err = astjson.MergeValuesWithPath(items[i], batch[idx], res.postProcessing.MergePath...) if err != nil { return errors.WithStack(ErrMergeResult{ Subgraph: res.ds.Name, Reason: err, Path: fetchItem.ResponsePath, }) } } }
♻️ Duplicate comments (2)
v2/pkg/engine/resolve/loader.go (2)
991-992: Extend error constants to cover unique-count mismatches and out-of-range indices.These distinguish batchStats path failures from 1:1 path failures and avoid ambiguous messages.
Apply this diff:
const ( failedToFetchNoReason = "" emptyGraphQLResponse = "empty response" invalidGraphQLResponse = "invalid JSON" invalidGraphQLResponseShape = "no data or errors in response" - invalidBatchItemCount = "returned items from batch do not match the number of items in the request. Expected %d, got %d" + invalidBatchItemCount = "returned items from batch do not match the number of items in the request. Expected %d, got %d" + invalidBatchUniqueItemCount = "returned items from batch do not match the number of unique items in the request. Expected %d, got %d" + invalidBatchIndexOutOfRange = "returned batch index %d is out of range for batch length %d" )
631-635: Add explicit bounds check for batch indices and use a distinct error for unique-count mismatches.The unique-index count equality check is good, but we also need to guard against out-of-range indices to avoid runtime panics (e.g., stats referencing an index >= len(batch)). Also, a dedicated error message for the “unique items” case improves clarity.
Apply this diff:
- uniqueIndexes := res.batchStats.getUniqueIndexes() - if uniqueIndexes != len(batch) { - return l.renderErrorsFailedToFetch(fetchItem, res, fmt.Sprintf(invalidBatchItemCount, uniqueIndexes, len(batch))) - } + uniqueIndexes, maxIdx := res.batchStats.uniqueIndexStats() + if uniqueIndexes != len(batch) { + return l.renderErrorsFailedToFetch(fetchItem, res, fmt.Sprintf(invalidBatchUniqueItemCount, uniqueIndexes, len(batch))) + } + if maxIdx >= len(batch) { + return l.renderErrorsFailedToFetch(fetchItem, res, fmt.Sprintf(invalidBatchIndexOutOfRange, maxIdx, len(batch))) + }
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/loader.go (2)
95-103: Name the skip sentinel and tighten docs.Using a named constant avoids repeating magic
-1. Also, nit: “JSON” capitalization.Apply this diff:
// batchStats represents an index map for batched items. -// It is used to ensure that the correct json values will be merged with the correct items from the batch. +// It is used to ensure that the correct JSON values will be merged with the correct items from the batch. // // Example: // // [[0],[1],[0],[1]] We originally have 4 items, but we have 2 unique indexes (0 and 1). // This means we are deduplicating 2 items by merging them from their response entity indexes. // 0 -> 0, 1 -> 1, 2 -> 0, 3 -> 1 type batchStats [][]int + +// batchIndexSkipped indicates that the item was skipped (error/null/empty). +const batchIndexSkipped = -1
1419-1470: Replace magic -1 with sentinel; optional: improve dedup structure and complexity.
- Use the new
batchIndexSkippedsentinel instead of raw-1when appending tores.batchStats.- Optional:
itemHasheslinear scan is O(n) per item; replace withindexByHash := map[uint64]intto bring it to amortized O(1). This can matter for larger batches.Apply this diff for the sentinel usage:
- err = nil // nolint:ineffassign - res.batchStats[i] = append(res.batchStats[i], -1) + err = nil // nolint:ineffassign + res.batchStats[i] = append(res.batchStats[i], batchIndexSkipped) @@ - if fetch.Input.SkipNullItems && buf.itemInput.Len() == 4 && bytes.Equal(buf.itemInput.Bytes(), null) { - res.batchStats[i] = append(res.batchStats[i], -1) + if fetch.Input.SkipNullItems && buf.itemInput.Len() == 4 && bytes.Equal(buf.itemInput.Bytes(), null) { + res.batchStats[i] = append(res.batchStats[i], batchIndexSkipped) continue } @@ - if fetch.Input.SkipEmptyObjectItems && buf.itemInput.Len() == 2 && bytes.Equal(buf.itemInput.Bytes(), emptyObject) { - res.batchStats[i] = append(res.batchStats[i], -1) + if fetch.Input.SkipEmptyObjectItems && buf.itemInput.Len() == 2 && bytes.Equal(buf.itemInput.Bytes(), emptyObject) { + res.batchStats[i] = append(res.batchStats[i], batchIndexSkipped) continue }Optional refactor sketch (not a diff to keep scope small):
// Replace: // itemHashes := make([]uint64, 0, len(items)) // for k := range itemHashes { if itemHashes[k] == itemHash { ... } } // With: indexByHash := make(map[uint64]int, len(items)) if k, ok := indexByHash[itemHash]; ok { res.batchStats[i] = append(res.batchStats[i], k) continue WithNextItem } indexByHash[itemHash] = batchItemIndex
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
v2/pkg/engine/resolve/loader.go(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T09:43:07.433Z
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.
Applied to files:
v2/pkg/engine/resolve/loader.go
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/loader.go (1)
v2/pkg/engine/resolve/fetch.go (1)
PostProcessingConfiguration(120-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (3)
v2/pkg/engine/resolve/loader.go (3)
652-655: Good: 1:1 length check moved to the non-batchStats path.This addresses the earlier false-negative scenario when deduplication/skip is active.
123-124: Struct field change looks correct.Switching to the
batchStatsalias keeps types coherent and localizes logic.
1425-1470: (I’ll wait for the script execution results to proceed with verification.)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
v2/pkg/grpctest/mockservice.go (1)
42-47: Avoid randomness in test data to prevent flakinessrand.Intn(100) makes assertions brittle. Prefer deterministic values.
Apply this diff:
- Location: fmt.Sprintf("Location %d", rand.Intn(100)), + Location: fmt.Sprintf("Location %s", warehouseId),v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (5)
346-352: Future-proof GraphQL error shape and de-duplicate response structsDefining a minimal error shape works, but tests may become brittle if the engine starts including path/locations/extensions. Also consider moving these helper types to package-level to reuse across tests in this file.
Apply this diff to enrich the error shape while keeping current tests green:
type graphqlError struct { - Message string `json:"message"` + Message string `json:"message"` + Path []any `json:"path,omitempty"` + Extensions map[string]any `json:"extensions,omitempty"` + Locations []struct { + Line int `json:"line"` + Column int `json:"column"` + } `json:"locations,omitempty"` } type graphqlResponse struct { Data map[string]interface{} `json:"data"` Errors []graphqlError `json:"errors,omitempty"` }
3504-3511: Name/readability: validateError is plural by contract and can be nil-guardedThe callback validates a slice of errors; pluralize the name and guard against nil to avoid accidental panics if a test omits the validator.
Minimal guard at call site (see Lines 3633-3634), and optionally rename here to validateErrors for clarity.
3569-3590: Make error assertion less brittle and assert count explicitlyAsserting exact error text ties the test to the current wording. Prefer asserting the salient parts and the error count to keep intent while reducing fragility.
- validateError: func(t *testing.T, errorData []graphqlError) { - require.NotEmpty(t, errorData) - require.Equal(t, "entity type Warehouse received 3 entities in the subgraph response, but 4 are expected", errorData[0].Message) - }, + validateError: func(t *testing.T, errorData []graphqlError) { + require.Len(t, errorData, 1) + require.Contains(t, errorData[0].Message, "entity type Warehouse") + require.Contains(t, errorData[0].Message, "3") + require.Contains(t, errorData[0].Message, "4") + },
3627-3634: Nil-guard the error validator to avoid accidental panics, and reuse enriched error typeYou already switched to graphqlResponse; add a guard when invoking the validator to keep tests resilient if future cases don’t supply a validator.
tc.validate(t, resp.Data) - tc.validateError(t, resp.Errors) + if tc.validateError != nil { + tc.validateError(t, resp.Errors) + }
159-160: Minor: fix typo in TODO and track actionable next stepNit: “anc” → “and”. If the intention is to verify no mapped response is returned, consider adding a dedicated test and linking an issue.
I can open a follow-up issue and scaffold the test that asserts an empty/missing response for unmapped calls—say, Test_DataSource_Load_Unmapped_NoResponse—if you want.
v2/pkg/grpctest/product.proto (1)
261-294: Docs mention “null in repeated result”; proto3 cannot encode null elementsThe guidance “Use null for entities that cannot be found” isn’t representable in a proto3 repeated message field. The engine can produce nulls at the GraphQL layer, but the gRPC response should either:
- Return a result element with zero-value message indicating not found, or
- Return the same count and let the JSON builder inject nulls based on an auxiliary presence signal.
Consider clarifying the comment to avoid misleading implementers of the service.
- * Always return the same number of entities as keys. Use null for entities that cannot be found. + * Always return the same number of entities as keys. + * For “not found”, return an empty/zero-value message at this layer; the GraphQL layer may translate that to null.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
v2/pkg/grpctest/productv1/product.pb.gois excluded by!**/*.pb.gov2/pkg/grpctest/productv1/product_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(3 hunks)v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go(2 hunks)v2/pkg/grpctest/mapping/mapping.go(2 hunks)v2/pkg/grpctest/mockservice.go(1 hunks)v2/pkg/grpctest/product.proto(3 hunks)v2/pkg/grpctest/schema.go(2 hunks)v2/pkg/grpctest/testdata/products.graphqls(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
v2/pkg/engine/plan/federation_metadata.go (1)
FederationFieldConfigurations(113-113)
v2/pkg/grpctest/schema.go (1)
pkg/introspection/introspection.go (1)
TypeName(42-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (9)
v2/pkg/grpctest/schema.go (2)
373-381: Add RootNodes metadata for Warehouse — aligned with schemaThe RootNodes entry for Warehouse (id, name, location) is correct and consistent with products.graphqls.
578-585: Add ChildNodes metadata for Warehouse — aligned with schemaChildNodes for Warehouse mirrors the type fields and will enable proper field access.
v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go (2)
242-251: EntityRPCs: Warehouse lookup wiring looks correctKeyed by id and bound to LookupWarehouseById request/response; matches the intended entity fetch path.
507-517: Fields map: Warehouse field mappings LGTMid, name, location target names align with proto/go types and other entities’ conventions.
v2/pkg/grpctest/mapping/mapping.go (2)
249-258: EntityRPCs: Warehouse added — confirm no QueryRPC is intendedGood addition for entity fetches. Please confirm we intentionally do not expose a Query-level Warehouse RPC (schema has no Query.warehouse).
514-524: Fields map: Warehouse fields mapped consistentlyThe mapping mirrors Storage; straightforward and correct.
v2/pkg/grpctest/mockservice.go (1)
22-53: Intentionally under-returning results is appropriate for testing invalid federated responsesThis directly supports the PR goal (“improve handling for invalid responses”). Ensure tests assert that the GraphQL layer surfaces a validation error when the batch size mismatches keys.
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
3564-3567: Good: asserts no GraphQL errors for the happy-path entity testClear assertion that the error payload is empty; aligns with the PR’s “invalid responses” focus.
v2/pkg/grpctest/product.proto (1)
635-640: Warehouse message LGTM; matches field patterns for Product/StorageField names and numbering are consistent and protobuf-safe. No issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
v2/pkg/engine/resolve/loader_test.go (3)
1150-1150: Use t.Cleanup to always verify gomock expectationsEnsures verification even if the test fails earlier and keeps style consistent with other tests that defer Finish.
func TestLoader_InvalidBatchItemCount(t *testing.T) { - ctrl := gomock.NewController(t) + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) @@ - ctrl.Finish()Also applies to: 1427-1427
1431-1433: Make assertions resilient to error orderingErrors from different fetch phases may appear in a different order over time. Compare messages order-independently and keep a lightweight data presence check.
- // 2 errors expected in the response. - expected := `{"errors":[{"message":"Failed to fetch from Subgraph, Reason: returned items from batch do not match the number of items in the request. Expected 3, got 2."},{"message":"Failed to fetch from Subgraph, Reason: returned items from batch do not match the number of items in the request. Expected 2, got 3."}],"data":{"topProducts":[{"name":"Table","__typename":"Product","upc":"1","reviews":[{"body":"Love Table!","author":{"__typename":"User","id":"1"}},{"body":"Prefer other Table.","author":{"__typename":"User","id":"2"}}]},{"name":"Couch","__typename":"Product","upc":"2","reviews":[{"body":"Couch Too expensive.","author":{"__typename":"User","id":"1"}}]},{"name":"Chair","__typename":"Product","upc":"3","reviews":[{"body":"Chair Could be better.","author":{"__typename":"User","id":"2"}}]}]}}` - assert.Equal(t, expected, out) + // 2 errors expected in the response (order-agnostic), and data present. + var parsed struct { + Errors []struct { + Message string `json:"message"` + } `json:"errors"` + } + assert.NoError(t, json.Unmarshal([]byte(out), &parsed)) + gotMsgs := make([]string, 0, len(parsed.Errors)) + for _, e := range parsed.Errors { + gotMsgs = append(gotMsgs, e.Message) + } + assert.ElementsMatch(t, []string{ + "Failed to fetch from Subgraph, Reason: returned items from batch do not match the number of items in the request. Expected 3, got 2.", + "Failed to fetch from Subgraph, Reason: returned items from batch do not match the number of items in the request. Expected 2, got 3.", + }, gotMsgs) + assert.Contains(t, out, `"topProducts":[`)
1429-1429: Redundant assert.NoErrorNo new error is set between the previous assertion and this line; this assert is redundant.
- assert.NoError(t, err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
v2/pkg/engine/resolve/loader_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/loader_test.go (9)
v2/pkg/engine/resolve/response.go (1)
GraphQLResponse(33-41)v2/pkg/engine/resolve/fetchtree.go (5)
Sequence(26-31)Single(59-67)Parallel(33-38)ArrayPath(52-57)ObjectPath(40-45)v2/pkg/engine/resolve/fetch.go (7)
SingleFetch(89-96)SingleFetch(155-157)FetchConfiguration(290-328)PostProcessingConfiguration(120-134)BatchEntityFetch(162-171)BatchEntityFetch(203-205)BatchInput(188-201)v2/pkg/engine/resolve/variables_renderer.go (1)
NewGraphQLVariableResolveRenderer(334-339)v2/pkg/engine/resolve/node_object.go (3)
Object(8-16)Object(30-32)Field(89-98)v2/pkg/engine/resolve/context.go (1)
Context(16-35)v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(69-77)ResolvableOptions(62-67)v2/pkg/engine/resolve/loader.go (1)
Loader(158-178)v2/pkg/fastjsonext/fastjsonext.go (1)
PrintGraphQLResponse(51-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (1)
v2/pkg/engine/resolve/loader_test.go (1)
1149-1433: Solid negative-path coverage for batched fetch mismatchesGood addition. The test exercises both under- and over-returned batch sizes, validates that errors surface in the GraphQL errors array, and that partial data still returns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
v2/pkg/engine/resolve/loader_test.go (4)
1150-1151: Always defer gomock Controller.Finish() to guarantee expectation verificationEnsures expectations are checked even if an assertion fails or the test panics before reaching the manual Finish call.
Apply this diff:
- ctrl := gomock.NewController(t) + ctrl := gomock.NewController(t) + defer ctrl.Finish() ... - ctrl.Finish()Also applies to: 1427-1427
1432-1432: Prefer JSON-aware equality for response comparisonUsing JSONEq avoids brittleness due to formatting or object-key ordering differences.
- assert.Equal(t, expected, out) + assert.JSONEq(t, expected, out)
1430-1430: Remove redundant assert on unchanged errThis assert re-checks the same err without re-assignment.
- assert.NoError(t, err)
1431-1433: Reduce coupling to exact error phrasing/order (optional)If error text changes slightly, this will fail. Consider also asserting substrings for the two counts to make intent explicit and order-agnostic.
For example (in addition to JSONEq or instead of full-body compare):
assert.Contains(t, out, "Expected 3, got 2.") assert.Contains(t, out, "Expected 2, got 3.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
v2/pkg/engine/resolve/loader.go(5 hunks)v2/pkg/engine/resolve/loader_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/resolve/loader.go
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/loader_test.go (10)
v2/pkg/engine/resolve/response.go (1)
GraphQLResponse(33-41)v2/pkg/engine/resolve/fetchtree.go (5)
Sequence(26-31)Single(59-67)Parallel(33-38)ArrayPath(52-57)ObjectPath(40-45)v2/pkg/engine/resolve/fetch.go (7)
SingleFetch(89-96)SingleFetch(155-157)FetchConfiguration(290-328)PostProcessingConfiguration(120-134)BatchEntityFetch(162-171)BatchEntityFetch(203-205)BatchInput(188-201)v2/pkg/engine/resolve/variables_renderer.go (1)
NewGraphQLVariableResolveRenderer(334-339)v2/pkg/engine/resolve/node_object.go (3)
Object(8-16)Object(30-32)Field(89-98)v2/pkg/engine/resolve/node_scalar.go (4)
String(48-54)String(91-93)Integer(230-234)Integer(236-238)v2/pkg/engine/resolve/context.go (1)
Context(16-35)v2/pkg/engine/resolve/resolvable.go (2)
NewResolvable(69-77)ResolvableOptions(62-67)v2/pkg/engine/resolve/loader.go (1)
Loader(158-178)v2/pkg/fastjsonext/fastjsonext.go (1)
PrintGraphQLResponse(51-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (1)
v2/pkg/engine/resolve/loader_test.go (1)
1149-1433: Good coverage for mismatched batch counts across parallel and sequential fetchesThe test clearly exercises both under- and over-return cases and validates surfaced GraphQL errors while preserving successful data. Looks solid.
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.222](v2.0.0-rc.221...v2.0.0-rc.222) (2025-08-27) ### Bug Fixes * improve handling for invalid responses ([#1283](#1283)) ([d656e91](d656e91)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of invalid responses, reducing errors and improving reliability in edge cases. * **Chores** * Bumped release version to 2.0.0-rc.222. * Updated changelog with latest fixes and release date. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist